-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimism: use op-alloy TxDeposit #10667
Conversation
new op-alloy patch out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last questions
pending @klkvr
crates/storage/codecs/src/lib.rs
Outdated
#[cfg(feature = "optimism")] | ||
use op_alloy_consensus as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I missed something with the imports but otherwise it triggers the unused_crate_dependencies
clippy (maybe a false positive). I have the impression that since op_alloy_consensus
is only used in a submodule under the optimism
cfg, it creates this weird warning...
However I thought I did things right in the Cargo.toml
with an optional and a dep only for the optimism
cfg..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo this is similar to what we have here
use op_alloy_rpc_types as _; |
crates/primitives/src/lib.rs
Outdated
#[cfg(feature = "optimism")] | ||
use reth_codecs as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this now necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason...
value: self.value, | ||
gas_limit: self.gas_limit as u64, | ||
is_system_transaction: self.is_system_transaction, | ||
input: self.input.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattsse currently all Compact
impls for alloy transactions clone input during encoding. should we address this with a helper struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine because this is Bytes
and should be cheap enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, pending @mattsse nits
Should close #9484
Waiting for the following inclusion in next op-alloy release:
decode_fields
pub forTxDeposit
alloy-rs/op-alloy#68TxDeposit
alloy-rs/op-alloy#69